-
Notifications
You must be signed in to change notification settings - Fork 6.9k
[core] upgrade opentelemetry-sdk #53745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
aef0d64 to
3109971
Compare
python/setup.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no longer needed since it's a part of default
c8c748e to
1ec2518
Compare
|
Also pending upgrade to the next release of vllm, which also removed ---amended by @lk-chen --- |
|
how recent is 1.30? |
|
i think fairly recent, 1.30 is feb 4, 2025; the current version installed in ray image is 1.26 was 1 year ago |
|
I'm a little concerned that such a recent version will cause dependency issues for our users. Do you have a sense of how easy it is to upgrade OT versions? Does it have many pinned transitive dependencies and/or breaking changes? |
ci/docker/serve.build.Dockerfile
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then this will break core telemetry? how do we make this stable?
I thought the plan is to drop the requirement on opentelemetry-exporter-otlp why is it still required here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @elliot-barn , our new dependency tzar (prospective)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discuss offline; will add comments about which packages are not currently resolved with opentelemetry-exporter-otlp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#updated
|
@edoakes your concern is completely valid, which is why I opened this as an RFC to gather feedback. Regarding your questions — I believe the upgrade should only be a problem for certain niche workloads. Here's why: Let’s assume a package is painful to upgrade primarily when it sits deep in the dependency chain — because updating it triggers a cascade of upgrades. Fortunately, opentelemetry-sdk/api sits at the top of the dependency chain in the Python ecosystem. That means not many packages depend on it directly. This is reflected in our requirements_compiled.txt, which includes many packages — but only a couple directly depend on opentelemetry-sdk/api:
Now, there are problematic package such as opentelemetry-exporter-otlp, which depends on opentelemetry-sdk/api and is known to be tricky to upgrade — particularly for users with tracing workloads, as it pulls in protobuf 5, which can introduce its own issues. On the other side, upgrading opentelemetry-sdk/api itself is also easy — it has a minimal set of dependencies (reference). |
4c17235 to
22db8a8
Compare
edoakes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on your evaluation I'm good with this change to unblock the OT migration.
Let's clearly document it in release notes with a call to action to open a GH issue if it causes any problems for users.
7b02c0a to
69c868d
Compare
c9fbbb9 to
f254406
Compare
f254406 to
9e58e61
Compare
27f2f37 to
c2e39a6
Compare
.buildkite/build.rayci.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC: @aslonnie, skipping this test as per offline discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context: the test is using an old, released wheel file but with current requirement_compiled.txt file.
REEf team will fix/refactor this test as a follow up
ci/docker/serve.build.Dockerfile
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you drop -U and add a version for opentelemetry-exporter-otlp ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this should be kept as the same version of opentelemetry-sdk and opentelemetry-api
Signed-off-by: can <can@anyscale.com>
| # Install tracing dependencies if requested. Intentionally, we do not use | ||
| # requirements_compiled.txt as the constraint file. They are not compatible with | ||
| # a few packages in that file (e.g. requiring an ugprade to protobuf 5+). | ||
| pip install opentelemetry-exporter-otlp==1.34.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pinned opentelemetry-exporter-otlp
This PR updates the `opentelemetry-sdk` version requirement to a minimum of 1.30, which is necessary to support histogram metrics as part of our migration from OpenCensus to OpenTelemetry. As part of this upgrade, I am also removing `opentelemetry-exporter-otlp` from the `ray[all]` dependency set. This package has been notoriously difficult to resolve alongside other dependencies, and this issue becomes even bigger with the updated SDK version. Since it is only used for the Ray tracing feature, I propose making it an optional, user-controlled dependency instead of a default one. Additionally, the test environment has been updated to ensure `test_tracing.py` continues to pass, verifying that tracing functionality remains intact. Test: - CI Signed-off-by: can <can@anyscale.com> Signed-off-by: jugalshah291 <shah.jugal291@gmail.com>
This PR updates the `opentelemetry-sdk` version requirement to a minimum of 1.30, which is necessary to support histogram metrics as part of our migration from OpenCensus to OpenTelemetry. As part of this upgrade, I am also removing `opentelemetry-exporter-otlp` from the `ray[all]` dependency set. This package has been notoriously difficult to resolve alongside other dependencies, and this issue becomes even bigger with the updated SDK version. Since it is only used for the Ray tracing feature, I propose making it an optional, user-controlled dependency instead of a default one. Additionally, the test environment has been updated to ensure `test_tracing.py` continues to pass, verifying that tracing functionality remains intact. Test: - CI Signed-off-by: can <can@anyscale.com> Signed-off-by: Douglas Strodtman <douglas@anyscale.com>
This PR updates the
opentelemetry-sdkversion requirement to a minimum of 1.30, which is necessary to support histogram metrics as part of our migration from OpenCensus to OpenTelemetry.As part of this upgrade, I am also removing
opentelemetry-exporter-otlpfrom theray[all]dependency set. This package has been notoriously difficult to resolve alongside other dependencies, and this issue becomes even bigger with the updated SDK version. Since it is only used for the Ray tracing feature, I propose making it an optional, user-controlled dependency instead of a default one.Additionally, the test environment has been updated to ensure
test_tracing.pycontinues to pass, verifying that tracing functionality remains intact.Test: